Update to rust-bitcoin v0.30.2 #2740

wpaulino · 2023-11-21T01:14:41Z

I tried to break up most of the changes into logical steps since there were so many things to fix, hopefully it helps.

A good follow-up to this would be to look into ScriptBuf uses that don't actually require the owned type.

tnull

Thanks for looking into this!

Seems tests are failing due to invalid PoWs in lightning-block-sync. I poked around a bit but couldn't immediately see what the issue is.

lightning/src/chain/channelmonitor.rs

lightning-transaction-sync/src/esplora.rs

lightning/src/ln/channelmanager.rs

lightning-block-sync/src/lib.rs

codecov-commenter · 2023-11-21T21:29:51Z

Codecov Report

Attention: 53 lines in your changes are missing coverage. Please review.

Comparison is base (870a0f1) 88.55% compared to head (ad56847) 88.50%.

Files	Patch %	Lines
lightning/src/chain/channelmonitor.rs	86.88%	5 Missing and 3 partials ⚠️
lightning-block-sync/src/convert.rs	77.77%	0 Missing and 4 partials ⚠️
lightning-invoice/src/lib.rs	63.63%	4 Missing ⚠️
lightning/src/chain/mod.rs	0.00%	4 Missing ⚠️
lightning/src/ln/channel.rs	88.88%	4 Missing ⚠️
lightning/src/sign/mod.rs	90.00%	2 Missing and 2 partials ⚠️
lightning-block-sync/src/init.rs	25.00%	3 Missing ⚠️
lightning/src/events/bump_transaction.rs	81.25%	3 Missing ⚠️
lightning-block-sync/src/poll.rs	60.00%	2 Missing ⚠️
lightning-block-sync/src/rest.rs	60.00%	2 Missing ⚠️
... and 12 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2740      +/-   ##
==========================================
- Coverage   88.55%   88.50%   -0.06%     
==========================================
  Files         113      113              
  Lines       89330    89323       -7     
  Branches    89330    89323       -7     
==========================================
- Hits        79110    79052      -58     
- Misses       7849     7896      +47     
- Partials     2371     2375       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tnull

LGTM from my side, I think.

Given the size of the changeset and its invasiveness having a second reviewer seems appropriate though.

lightning-block-sync/src/test_utils.rs

valentinewallace

Whew, LGTM. Good to squash if we want to fix check-commits.

wpaulino · 2023-11-22T23:51:54Z

Squashed and found a way to drop one of the commits (wpaulino@9857df3).

valentinewallace

Someone feel free to land this after CI passes if I'm AFK, since @tnull also ack'd previously.

lightning-block-sync/src/poll.rs

lightning/src/chain/channelmonitor.rs

tnull

LGTM! CI failure is unrelated, so I'm going ahead landing this.

Kixunil

Congrats on merging this! I took a look to see what we can improve in bitcoin and found some inspiration and also potential improvements on your side.

Kixunil · 2023-11-23T16:58:22Z

lightning-block-sync/src/convert.rs

 			let BlockHeaderData { chainwork, height, header } = data;
 			serde_json::json!({
-				"chainwork": chainwork.to_string()["0x".len()..],
+				"chainwork": chainwork.to_be_bytes().as_hex().to_string(),


FYI this could be format!("{:064x}", chainwork) which conveys the intent better.

Kixunil · 2023-11-23T17:08:16Z

lightning-block-sync/src/convert.rs

 				"nonce": header.nonce,
-				"bits": header.bits.to_hex(),
-				"previousblockhash": header.prev_blockhash.to_hex(),
+				"bits": header.bits.to_consensus().to_be_bytes().as_hex().to_string(),


format!("{08x}", header.bits.to_consensus()) would convey the meaning better but your code is actually faster because std formatting is over-complicated. I've opened an issue to support hex directly on CompactTarget but that will still be slower than your code.

Kixunil · 2023-11-23T17:20:49Z

lightning-invoice/src/lib.rs

+					match WitnessProgram::new(*version, program.clone()) {
+						Ok(witness_program) => Payload::WitnessProgram(witness_program),
+						Err(_) => return None,
+					}


So previously you'd accept invalid addresses but now you silently ignore them. Is this even correct? Maybe the error handling should be at creation of Fallback::SegWitProgram, so the variant should change to store WitnessProgram directly?

These are parsing invoices provided by random scanning. We'd much rather succeed at parsing an invoice with invalid fallback addresses than ignore it, and better to provide some set of fallback addresses than nothing. That said, I'm pretty sure ~no one actually uses the fallback addresses in BOLT11, since unified QR codes via BIP 21 are a thing now.

Perhaps add an API to check if any are invalid so that an application can emit a warning in such case?

Given these things are basically unused, I'm not sure its worth any effort :)

True, I think it should be at least be documented. I'd be pissed if a library silently dropped data even in edge cases.

Kixunil · 2023-11-23T17:33:06Z

lightning/src/chain/channelmonitor.rs

 				}
 			}
-			assert_eq!(base_weight + inputs_total_weight as usize,  claim_tx.weight() + /* max_length_sig */ (73 * inputs_weight.len() - sum_actual_sigs));
+			assert_eq!(base_weight + inputs_total_weight, claim_tx.weight().to_wu() + /* max_length_sig */ (73 * inputs_weight.len() as u64 - sum_actual_sigs));


It looks like this could be somehow replaced with predict_weight(https://docs.rs/bitcoin/latest/bitcoin/blockdata/transaction/fn.predict_weight.html) but the whole code is complicated so I'm not sure what it does.

Anyway, in general we want to avoid operations on raw integers so it might be nice to understand your use case better. Note that Weight doesn't impl Add precisely because it breaks naive implementations once there's more than 252 inputs/outputs. I don't see such handling in your code so you might have the same bug here?

We have a few places where we'd benefit from a weight estimation/prediction API. Now that there's one in rust-bitcoin, we should definitely switch over, just didn't want to handle that in this PR.

Kixunil · 2023-11-23T17:39:17Z

lightning/src/ln/channel_id.rs

 	fn test_channel_id_v1_from_funding_txid() {
 		let channel_id = ChannelId::v1_from_funding_txid(&[2; 32], 1);
-		assert_eq!(channel_id.to_hex(), "0202020202020202020202020202020202020202020202020202020202020203");
+		assert_eq!(channel_id.0.as_hex().to_string(), "0202020202020202020202020202020202020202020202020202020202020203");


Why not channel_id.to_string()?

Kixunil · 2023-11-23T17:44:31Z

lightning/src/ln/peer_channel_encryptor.rs

-		let our_node_id = SecretKey::from_slice(&hex::decode("2121212121212121212121212121212121212121212121212121212121212121").unwrap()[..]).unwrap();
-		let our_ephemeral = SecretKey::from_slice(&hex::decode("2222222222222222222222222222222222222222222222222222222222222222").unwrap()[..]).unwrap();
+		let our_node_id = SecretKey::from_slice(&<Vec<u8>>::from_hex("2121212121212121212121212121212121212121212121212121212121212121").unwrap()[..]).unwrap();
+		let our_ephemeral = SecretKey::from_slice(&<Vec<u8>>::from_hex("2222222222222222222222222222222222222222222222222222222222222222").unwrap()[..]).unwrap();


Looks like hex_lit::hex macro would help with these.

wpaulino added this to the 0.0.119 milestone Nov 21, 2023

tnull reviewed Nov 21, 2023

View reviewed changes

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

lightning-transaction-sync/src/esplora.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Show resolved Hide resolved

lightning-block-sync/src/lib.rs Outdated Show resolved Hide resolved

wpaulino force-pushed the rust-bitcoin-30-update branch 2 times, most recently from 2930eb9 to 8fc4764 Compare November 21, 2023 21:23

wpaulino force-pushed the rust-bitcoin-30-update branch from 8fc4764 to 768b975 Compare November 21, 2023 23:28

This was referenced Nov 22, 2023

Bump MSRV to rustc 1.63.0 #2681

Merged

Add electrum support to lightning-transaction-sync #2685

Merged

tnull reviewed Nov 22, 2023

View reviewed changes

lightning-block-sync/src/test_utils.rs Outdated Show resolved Hide resolved

valentinewallace reviewed Nov 22, 2023

View reviewed changes

wpaulino force-pushed the rust-bitcoin-30-update branch from 768b975 to 8220621 Compare November 22, 2023 23:50

valentinewallace previously approved these changes Nov 22, 2023

View reviewed changes

arik-so reviewed Nov 22, 2023

View reviewed changes

lightning-block-sync/src/poll.rs Show resolved Hide resolved

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

wpaulino dismissed valentinewallace’s stale review via 6181985 November 22, 2023 23:57

wpaulino force-pushed the rust-bitcoin-30-update branch from 8220621 to 6181985 Compare November 22, 2023 23:57

wpaulino added 2 commits November 22, 2023 15:58

Bump rust-bitcoin to v0.30.2

ec928d5

Remove nightly warnings

ad56847

wpaulino force-pushed the rust-bitcoin-30-update branch from 6181985 to ad56847 Compare November 22, 2023 23:58

valentinewallace approved these changes Nov 22, 2023

View reviewed changes

tnull approved these changes Nov 23, 2023

View reviewed changes

tnull merged commit 70ea110 into lightningdevkit:main Nov 23, 2023

tnull mentioned this pull request Nov 23, 2023

Bump rust-bitcoin to version 0.31 #2745

Closed

Kixunil reviewed Nov 23, 2023

View reviewed changes

wpaulino deleted the rust-bitcoin-30-update branch November 27, 2023 18:44

Update to rust-bitcoin v0.30.2 #2740

Update to rust-bitcoin v0.30.2 #2740

Uh oh!

Conversation

wpaulino commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

wpaulino commented Nov 22, 2023

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

wpaulino commented Nov 21, 2023 •

edited

Loading

codecov-commenter commented Nov 21, 2023 •

edited

Loading